-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(odyssey-react-mui): add overline typography variant #2349
Conversation
}, | ||
render: (args) => <Overline {...args} children={args.children} />, | ||
play: async ({}) => { | ||
await axeRun("Typopgraphy Overline"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await axeRun("Typopgraphy Overline"); | |
await axeRun("Typography Overline"); |
}, | ||
render: (args) => <Legend {...args} children={args.children} />, | ||
play: async ({}) => { | ||
await axeRun("Typopgraphy legend"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await axeRun("Typopgraphy legend"); | |
await axeRun("Typography legend"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"html:lang(en) &": { | ||
textTransform: "uppercase", | ||
}, | ||
|
||
"html:lang(en-*) &": { | ||
textTransform: "uppercase", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this instead since we always want these to share changes?
"html:lang(en) &": { | |
textTransform: "uppercase", | |
}, | |
"html:lang(en-*) &": { | |
textTransform: "uppercase", | |
}, | |
"html:lang(en) &, html:lang(en-*) &": { | |
textTransform: "uppercase", | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinGhadyani-Okta Strangely, this doesn't work. I had to make 2 declarations for this to work correctly. I don't know why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to combine these selectors in quite a few ways to get it working. No luck
import { Typography } from "./Typography"; | ||
|
||
describe("Typography", () => { | ||
test("renders overline", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test("renders overline", () => { | |
test("renders Overline", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding unit tests! Not much to test in this component since it's visual, and VRTs will cover the other half of it 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved from a dev perspective. I want someone from design to look at the Storybook and approve as well.
@KevinGhadyani-Okta Where do we find the Storybook preview URL now? |
41dd73e
to
2ca80a5
Compare
OKTA-803564
Summary
Overline
typography componentTesting & Screenshots